-
Notifications
You must be signed in to change notification settings - Fork 47
Changes needed to support ONIX #360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
for more information, see https://pre-commit.ci
| custom_parameters = editor.find("CUSTOM_PARAMETERS") | ||
| np_probes = custom_parameters.findall("NP_PROBE") | ||
| if onix_processor is not None: | ||
| np_probes = custom_parameters.findall("NEUROPIXELSV1E") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will NEUROPIXELSV1E always be the name of the probe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be V1 or V2 depeneding on Neuropixels1.0/2.0. @bparks13 could you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the available options are NEUROPIXELSV1E, NEUROPIXELSV1F, and NEUROPIXELSV2E. The V1E/V1F are both Neuropixels 1.0 probes, and can be used interchangeably at this stage, the E/F distinction is differentiating the other ONIX hardware/headstage.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
==========================================
+ Coverage 89.92% 90.11% +0.18%
==========================================
Files 12 12
Lines 2044 2083 +39
==========================================
+ Hits 1838 1877 +39
Misses 206 206 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if onix_processor is not None: | ||
| slot, port, dock = None, None, None | ||
| probe_part_number, probe_serial_number = np_probe.attrib["probePartNumber"], np_probe.attrib["probeSerialNumber"] | ||
| channels = np_probe.find("SELECTED_CHANNELS") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, the names of several settings are slightly different than the other cases. e.g. ONIX uses probePartNumber instead of probe_part_number.
Also, I don't think ONIX stores the slot, port and dock? Or maybe PortB in the probe_names_used says which port is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of naming, some of these elements have been introduced in the draft PR here, which gives us some flexibility with changing the names. I could modify the name to be probe_part_number if that makes more sense to be consistent.
Since we are recording from Open Ephys hardware, and not IMEC hardware, we do not have a slot/port/dock to report. The Port in this instance is specifying which port on the Breakout Board the headstage is connected to.
| xpos = np.array([float(electrode_xpos.attrib[ch]) for ch in channel_names]) | ||
| ypos = np.array([float(electrode_ypos.attrib[ch]) for ch in channel_names]) | ||
| positions = np.array([xpos, ypos]).T | ||
| else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the hardest thing: electrode positions are not saved with ONIX, but they are for the other formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I think that the easiest would be to store the electrode x/y positions, or the selected electrods. In the latter case, we could instantiate a full NP probe from the probe part number and slice it accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are currently storing the selected electrodes in the settings.xml file, under the SELECTED_CHANNELS node. The indices are the global electrode index. I would be happy to talk through how it is currently set up, and we can determine if that information is enough or I can add in more details as needed.
|
@chrishalcrow @alejoe91 I have added some additional zipped folders containing some more diverse electrode configurations in the plugin PR: NPX 1.0 configurations NPX 2.0 configurations As per our discussion, you should not need the JSON files that are in the zipped folders, but the settings.xml files contain the selected electrode index and the probe part number, which should be sufficient. The JSON files will still be kept there for ease of use for the user, but are not needed in this PR. Let me know if you need anything else from me! |
|
Hey @bparks13 - thanks for all the new data. So when you have one probe the structure is e.g. and the probe information is stored in the And when you have two or more it's e.g. and the probe information is stored in the Is that right? Are the names |
|
@chrishalcrow The names are related to the NP version, as you mentioned. The difference in the structure is related to the physical hardware we use. Some headstages (NeuropixelsV1e/NeuropixelsV1f) have one probe per device, while NeuropixelsV2e headstages have one device with two probes:
I hope this helps! Let me know if anything doesn't make sense. I'm happy to meet and discuss it in more detail as well. |
|
Amazing - very helpful! |
|
@alejoe91 Here is a link where you can find the configurations with a single-shank probe, that correctly stays within its electrode boundaries! open-ephys-plugins/onix-source#152 (comment) Let me know if you need anything else, but it looks like once these are added to the tests it should be good to go. |
|
@chrishalcrow @bparks13 can we merge this? LGTM now, but let's have a final look |
Co-authored-by: Chris Halcrow <[email protected]>
|
@alejoe91 everything looks good to me, the only thing I want to confirm is if the @chrishalcrow also pointed out a |
Yes! This is already the case. We recently refactored the mechanism because there was an issue when the user chose to save a subset of channels. But it will be fixed in the next release. For reference, this is the function that gets sample shifts from the probe: https://github.com/SpikeInterface/spikeinterface/blob/main/src/spikeinterface/extractors/neuropixels_utils.py#L10 |
This PR shows what changes would be needed to implement ONIX support directly using
read_openehys. This would avoid having to make a separate.jsonfile and you get mux tables etc for free! It's a messy PR, just to see what the pain points are. I've written comments where I'm confused/unsure about something.Test data: open-ephys-plugins/onix-source#144 (comment)